Allow explicit marking of local variables as the key for merge explosion with CompilerDirectives.mergeExplodeKey
#12557
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
gilles-duboscq
left a comment
gilles-duboscq
left a comment
There was a problem hiding this comment.
Please also check the 2 failed gates
| * @param i the variable that should be used as the merge key. | ||
| * @return the unchanged value | ||
| */ | ||
| @SuppressWarnings("unused") |
| * the value must be a constant integer. | ||
| * | ||
| * @param i the variable that should be used as the merge key. | ||
| * @return the unchanged value |
| @Override | ||
| public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Receiver receiver, ValueNode value) { | ||
| if (canDelayIntrinsification) { | ||
| return false; |
There was a problem hiding this comment.
I think this deserves a comment explaining why we do this.
| import jdk.graal.compiler.nodes.spi.CanonicalizerTool; | ||
|
|
||
| @NodeInfo(nameTemplate = "LoopExplosionKey", cycles = CYCLES_0, size = SIZE_0) | ||
| public final class LoopExplosionKeyNode extends FloatingNode implements Canonicalizable { |
There was a problem hiding this comment.
What happens if the directive is used in a non-merge-explode method and this node remains in the graph?
I think this should either be a very explicit compilation failure or performance warning. If it's only a warning then we need to remove such nodes.
@chumer what do you think? Should using CompilationDirective.mergeExplodeKey outside of the context of a @ExplodeLoop(kind = LoopExplosionKind.MERGE_EXPLODE) method be a compilation error or a performance warning?
| } | ||
| } | ||
|
|
||
| graph.maybeMarkUnsafeAccess(encodedGraph); |
There was a problem hiding this comment.
Why did you remove this?
| LoopExplosionState queryState = new LoopExplosionState(frameState, null); | ||
| LoopExplosionState existingState = loopScope.iterationStates.get(queryState); | ||
| if (loopScope.trigger == LoopScopeTrigger.START) { | ||
| List<Integer> mergeKey = null; |
There was a problem hiding this comment.
| List<Integer> mergeKey = null; | |
| List<Integer> mergeKeys = null; |
| for (int i : loopScope.loopExplosionMergeKeySlots) { | ||
| ValueNode node = frameState.values().get(i); | ||
| if (!node.isConstant() || node.asJavaConstant().getJavaKind() != JavaKind.Int) { | ||
| throw new PermanentBailoutException("Graal implementation restriction: Method with %s loop explosion must have a loop variable of type int. %s", |
There was a problem hiding this comment.
| throw new PermanentBailoutException("Graal implementation restriction: Method with %s loop explosion must have a loop variable of type int. %s", | |
| throw new PermanentBailoutException("Graal implementation restriction: merge keys must partial evaluate to int constants at the loop header. %s", |
| values = new ArrayList<>(loopScope.loopExplosionMergeKeySlots.size()); | ||
| for (int i : loopScope.loopExplosionMergeKeySlots) { | ||
| ValueNode node = frameState.values().get(i); | ||
| if (!node.isConstant() || node.asJavaConstant().getJavaKind() != JavaKind.Int) { |
There was a problem hiding this comment.
Should we accept all integer types (i.e., also accept longs)?
What do you think @chumer?
| // merge has a frame state | ||
| loop.exits.removeIf(x -> x.merge() == merge); | ||
| loop.exits.add(end); | ||
| continue; |
There was a problem hiding this comment.
this continue doesn't look necessary
| livenessAnalysis.onStart(frame, skipLivenessActions); | ||
| } | ||
|
|
||
| curBCI = CompilerDirectives.mergeExplodeKey(curBCI); |
There was a problem hiding this comment.
This could be moved to a separate commit
2 participants
This PR adds a method
CompilerDirectives.mergeExplodeKeywhich can be used to explicitly mark variables to be used as the merge key inMERGE_EXPLODEloop explosion.Currently, the entire frame state is used for merging. Taking Espresso as an example, if you would jump to the same
bcibut assign a different value to another variable, it will result in two separate code paths. This can lead to unexpected graph size explosion and bugs. With the method introduced in this PR, you can mark a single variable to be used as the key. Truffle will merge on this key only, but verify that other variables stay the same. If merging should occur but there is a mismatch, the compilation will bail out.Future steps include allowing multiple variables to be marked simultaneously (which has to be extended into irreducible loop handling), and creating an option for explicit PHI nodes, so local variables can be used as counters without the need for objects.